Conversation
lukewarlow
force-pushed
the
update-EnsureCSPDoesNotBlockStringCompilation
branch
from
23208bd to
7613b69
Compare
annevk
left a comment
annevk
left a comment
There was a problem hiding this comment.
Why is bodyString passed to CSP if it's completely ignored?
|
Mentioned on the HTML discussion but just so it's easily findable here to, CSP has future plans to make use of it (see #623 for the use case) |
|
A bigger problem is that HTML doesn't pass in source. |
Yeah I can't speak for past reasoning but it seems pretty wild that Ecmascript didn't just update to include the full source back in the day when CSP needed it. And it seems CSP went with the approach of just saying well we need it so you as the implementor have to figure that bit out. I guess we can wait and see what happens to ECMAScript, or we can update HTML to pass it through and then it's just ecmascript that needs dealing with (though trusted types requires extra params too)? I'll defer to your editorial experience on the best path forwards :) |
|
I think we should start with ECMAScript. They take PRs too. 😊 |
…sureCanCompileStrings definition.
lukewarlow
force-pushed
the
update-EnsureCSPDoesNotBlockStringCompilation
branch
from
7613b69 to
961e959
Compare
|
|
||
| 1. If |compilationType| is `*FUNCTION*`: | ||
|
|
||
| 1. Set |source| to `"function anonymous("` |
There was a problem hiding this comment.
TC39 didn't approve passing through the compiled code string so instead we reconstruct it here.
This matches WebKit and Firefox but Chromium currently wraps the entire string in () brackets.
This also doesn't have the context of whether it's an async function or generator, so this is a change in that regard but the usage of their constructors is probably very low (cc @nicolo-ribaudo)
There was a problem hiding this comment.
Can you link the discussion?
Not sure who has to approve this for Chromium. @andypaicu and @mikewest maybe?
There was a problem hiding this comment.
Sorry for the delay on getting back to this. The discussion isn't published yet (I think soon?).
After a bit of a back and forth it turns out the reasoning behind the objection was flawed and so we're going to ask (I don't forsee there being push back personally) them to change it in the next plenary. In the mean time I'll change this spec PR (and HTMLs) to assume it does get passed through. Given CSP at least is already "broken" in that regard it doesn't seem bad to continue it for a bit longer till we get the tc39 side updated.
There was a problem hiding this comment.
Given this is well defined in ECMA262 it would be interesting to see where the discrepency in Chrome comes from and whether there's test coverage for that particular aspect.
| ISSUE(tc39/ecma262#938): {{HostEnsureCanCompileStrings()}} does not include the string which is | ||
| going to be compiled as a parameter. We'll also need to update HTML to pipe that value through | ||
| to CSP. | ||
| Note: |parameterStrings|, |parameterArgs|, |bodyArg| and |bodyString| are currently unused. They are included for future use. |
There was a problem hiding this comment.
parameterStrings, codeString, compilationType, parameterArgs, and bodyArg are currently unused. Right?
There was a problem hiding this comment.
codeString is used but compilationType isn't I've updated that.
Update the HostEnsureCanCompileStrings definition to match dynamic code brand checks stage 3 proposal. Also update the call to EnsureCSPDoesNotBlockStringCompilation to pass these new arguments through. Also update the timer initialization steps to call EnsureCSPDoesNotBlockStringCompilation directly, and include the new parameters. Also define HostGetCodeForEval implementation. See w3c/webappsec-csp#650 for corresponding CSP PR. Also see #10202 for context.
Update the HostEnsureCanCompileStrings definition to match dynamic code brand checks stage 3 proposal. Also update the call to EnsureCSPDoesNotBlockStringCompilation to pass these new arguments through. Also update the timer initialization steps to call EnsureCSPDoesNotBlockStringCompilation directly, and include the new parameters. Also define HostGetCodeForEval implementation. See w3c/webappsec-csp#650 for corresponding CSP PR. Also see whatwg#10202 for context.
2 participants
See https://tc39.es/proposal-dynamic-code-brand-checks/#sec-hostensurecancompilestrings for latest version of the Host hook.
See also whatwg/html#10204